Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: module imports should not use type index as their function identifier #411

Merged
merged 2 commits into from
Jul 18, 2018
Merged

fix: module imports should not use type index as their function identifier #411

merged 2 commits into from
Jul 18, 2018

Conversation

ColinEberhardt
Copy link
Collaborator

@ColinEberhardt ColinEberhardt commented Jul 13, 2018 •
edited
Loading

fixes #406

@xtuc there is a wasm-edit test failing as a result of this change, wondering if you have any pointers?

ColinEberhardt
ColinEberhardt commented Jul 13, 2018
View reviewed changes
packages/wasm-parser/test/fixtures/import/complex/actual.wat
@@ -0,0 +1,5 @@
(module
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this simple test, which adds a couple of type definitions, was enough to repro the issues from #406

ColinEberhardt
ColinEberhardt commented Jul 13, 2018
View reviewed changes
packages/wast-parser/src/grammar.js
@@ -419,6 +417,8 @@ export function parse(tokensList: Array<Object>, source: string): Program {
const fnParams = [];
const fnResult = [];

let fnName = t.identifier(getUniqueName("func"));
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change was not necessarily needed to make tests pass, but this does mean we have the same generated names when parsing wat

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following https://github.com/xtuc/webassemblyjs/pull/411/files#r202406723 we could add our own func increment, or maybe we already have this information?

@xtuc
Copy link
Owner

xtuc commented Jul 13, 2018

The "AST synchronization" test suite in wasm-edit ensure that we mutate the AST correctly by comparing it to a freshly decoded AST.

xtuc
xtuc reviewed Jul 13, 2018
View reviewed changes
packages/wasm-parser/src/decoder.js
@@ -432,7 +432,7 @@ export function decode(ab: ArrayBuffer, opts: DecoderOpts): Program {
throw new CompileError(`function signature not found (${typeindex})`);
}

const id = t.numberLiteralFromRaw(typeindex);
const id = getUniqueName("func");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should try to eliminate the unique name generator (because statefull), and rather use the typeindex here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should try to eliminate the unique name generator (because statefull)

I agree, that maybe the type name generator should be removed. I'm not sure what it's used for!

and rather use the typeindex here

No ... that's the bug that this PR is fixing. Module imports are functions which are referenced by index, i.e. if you have the following:

(type (func (result i32)))
(type (func (param i32) (result i32)))
(import "a" "c" (func $foo (type 1)))
(func $bar)

The function foo has index 0, and bar has index 1. The index does not relate to the type reference.

@ColinEberhardt
Copy link
Collaborator Author

The "AST synchronization" test suite in wasm-edit ensure that we mutate the AST correctly by comparing it to a freshly decoded AST.

OK - I'll explore further and see what I've broken!

@ColinEberhardt
Copy link
Collaborator Author

The "AST synchronization" test suite in wasm-edit ensure that we mutate the AST correctly by comparing it to a freshly decoded AST.

@xtuc ok, I've had a look at the test and understand what it is doing now. I've fixed the test by making the following changes:

  • the makeFuncNodes function was adding the AST for the function type as well as the function itself. I've moved the construction of function types into a separate step as the module imports use this too
  • I've changed the order, to add the module import first
  • following this, the test adds a function, which now has an index of 1, and an identifier of func_1. As discussed in refactor index #409 - this is something that we'll probably remove in future, but for now, this fixes the original issue where type indices were being used as function indices.

@ColinEberhardt
Copy link
Collaborator Author

The tests are currently failing, but looks like a CI issue rather than a unit test failure!

xtuc
xtuc approved these changes Jul 18, 2018
View reviewed changes
Copy link
Owner

@xtuc xtuc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updates!

@ColinEberhardt
Copy link
Collaborator Author

kicked Travis and the build works now :-)

@ColinEberhardt ColinEberhardt merged commit 1d212df into master Jul 18, 2018
@xtuc xtuc deleted the fix-module-import-id branch July 18, 2018 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xtuc xtuc xtuc approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

wasm-parser -- Decode can produce unexpected module import identifier
2 participants
@ColinEberhardt @xtuc